-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lnwallet: allow opener to dip into its reserve. #9100
base: master
Are you sure you want to change the base?
lnwallet: allow opener to dip into its reserve. #9100
Conversation
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Looking for a Concept ACK here and also an Approach ACK before adding tests. |
What should be taken into account when deciding whether we should go with this change:
So I kinda tend to go straight to Zero-FeeCommitments or even Anchor Channels with no FeeUpdates as soon as Bitcoind 28 is rolled out widely. Curios what you all think about this. |
That will take a very long time before it's available: I think it's worth improving the reserve requirements today to avoid more force-closed channels (I suspect some force-closed channels happen because of races between The most important thing to release ASAP is to ensure that as the channel opener, you allow receiving HTLCs that make you dip into your reserve. From the channel opener's point of view, it's a 100% win: you don't have anything to lose by dipping into your own channel reserve, it is your peer that may be taking a risk when that happens. We had discussed that behavior a long time ago in spec meetings, and all implementations had committed to ship this behavior, but I'm not sure lnd has actually shipped it since, so it's really time to do it?
If you haven't yet shipped the behavior described above, you may indeed want to only allow this when you think the remote node supports it. That can be implicitly done with good enough accuracy by checking for a new (unrelated) feature that would be added in the same release as these new reserve checks? |
Thanks for taking the time @t-bast. Great I totally agree we should ship the case where we allow incoming HTLCs as the opener to be more in line with other implementations.
Need to think about a good way to also enable sending support in terms of backwards compatibility, your proposal is definitely one. |
I agree we should allow dipping into our reserve as an HTLC receiver. I'm not sure about letting the other party dip into their reserve to pay commitment fees though... How often do these stuck channels happen? Perhaps we should increase the fee spike buffer instead... |
5769cfd
to
92c5ac9
Compare
Fixes: #8249
See also ACINQ/eclair#2899 for more background.
Although zero-fee commitments are on the horizon it will be probably take longer until all new channels are upgraded to this new type where we basically don't need those exceptions anymore. With the current channel type we need this exception to make splicing work and also protect against some force closes with other node implementation e.g. eclair which excepts the dipping into the reserve in some case.
There are some cases where we should allow the peer paying the channel fees (channel opener) to be dipped into its reserve. These will be detailed in the following to have an easier way to understand this PR.
We need to look at this problem from the perspective of the different messages which are allowed to change the channel state in Lightning causing the commitment fees to potentially increase. (Update_FullFill and Update_Fail are not mentioned here because they do not increase the fee requirements but only resolve htlcs from the commitment).
Now we need to look at those messages from the perspective of the channel opener paying the fees and the non-opener.
In general ONLY the peer paying the fees is allowed to be dipped into its reserve. A non-opener can be below the reserve (when a channel without push_amt is opened) but the balance should only increase not decrease hence its important to notice that we are only looking into the cases where the initial balance of one side decreased.
Channel Opener:
Looking into the case when the local reserve is allowed to be dipped.
Channel Non-Opener.
We look now when the remote reserve is allowed to be dipped.